Implement lazy addition #9
Conversation
| @eval function materialize!(M::$MulAdd_{<:BroadcastLayout{typeof(+)}}) | ||
| α, A, B, β, C = M.α, M.A, M.B, M.β, M.C | ||
| if C ≡ B | ||
| B = copy(B) |
There was a problem hiding this comment.
I noticed that you are testing B .= Mul(A,B) so I added this branch but is it a good idea? First, aliasing can happen through view etc. so using Base.mightalias would be safer? Also, wouldn't it be better to just error out if we can't guarantee it is allocation-free?
There was a problem hiding this comment.
is it a good idea?
I'm not sure. I think I added that line just for completeness, I don't think we necessary need it.
so using Base.mightalias would be safer?
Yes, but when I wrote that line I didn't know about mightalias.
Also, wouldn't it be better to just error out if we can't guarantee it is allocation-free?
Possibly. I believe broadcasting has an unaliascopy for this situation, but this does allocate... I'm happy with either behaviour so maybe its safest to throw an error until someone complains about it?
There was a problem hiding this comment.
broadcasting has an
unaliascopyfor this situation, but this does allocate
Ah, yes. So throwing error is incompatible with standard broadcasting behavior... I missed that point. I wonder what was the reasoning for unaliascopy fallback. Maybe it was something like "if you benchmark it you find the GC".
There was a problem hiding this comment.
We can change this here and elsewhere to use unaliascopy at a later date.
Codecov Report
@@ Coverage Diff @@
## master #9 +/- ##
==========================================
+ Coverage 47.96% 49.74% +1.77%
==========================================
Files 11 11
Lines 761 772 +11
==========================================
+ Hits 365 384 +19
+ Misses 396 388 -8
Continue to review full report at Codecov.
|
| @@ -76,3 +90,14 @@ broadcasted(::LazyArrayStyle{N}, ::typeof(*), a::AbstractArray{T,N}, b::Zeros{V, | |||
| broadcast(DefaultArrayStyle{N}(), *, a, b) | |||
| broadcasted(::LazyArrayStyle{N}, ::typeof(*), a::Zeros{T,N}, b::AbstractArray{V,N}) where {T,V,N} = | |||
| broadcast(DefaultArrayStyle{N}(), *, a, b) | |||
|
|
|||
| const Add = BroadcastArray{<:Any, <:Any, <:Broadcasted{<:Any, <:Any, typeof(+)}} | |||
There was a problem hiding this comment.
Add is an AbstractMarix so maybe AddArray is better as it would be consistent with MulArray? (Though I prefer seeing Add in my code than AddArray or AddMatrix.)
There was a problem hiding this comment.
Hmm, this is a tough one. What we could do is the following:
struct AddStyle <: BroadcastStyle end
const Add = Broadcasted{<:AddStyle, <:Any, typeof(+)}
Add(A...) = Broadcasted{AddStyle}(+, A)where the AddStyle means it's not type piracy.
But I'm not convinced Add should exist at all: what about Minus? Is Add(A,B) that much better than, broadcasted(+, A, B), which also forces it to not materialize?
There was a problem hiding this comment.
So maybe just use BroadcastArray like in this PR at the moment?
ref: #8 (comment)
src/LazyArrays.jl
Outdated
| @@ -38,7 +38,7 @@ import FillArrays: AbstractFill | |||
|
|
|||
| import StaticArrays: StaticArrayStyle | |||
|
|
|||
| export Mul, MulArray, MulVector, MulMatrix, | |||
| export Mul, MulArray, MulVector, MulMatrix, Add, | |||
There was a problem hiding this comment.
Let's not export Add in case we decide to change its definition.
| @@ -76,3 +90,14 @@ broadcasted(::LazyArrayStyle{N}, ::typeof(*), a::AbstractArray{T,N}, b::Zeros{V, | |||
| broadcast(DefaultArrayStyle{N}(), *, a, b) | |||
| broadcasted(::LazyArrayStyle{N}, ::typeof(*), a::Zeros{T,N}, b::AbstractArray{V,N}) where {T,V,N} = | |||
| broadcast(DefaultArrayStyle{N}(), *, a, b) | |||
|
|
|||
| const Add = BroadcastArray{<:Any, <:Any, <:Broadcasted{<:Any, <:Any, typeof(+)}} | |||
| @eval function materialize!(M::$MulAdd_{<:BroadcastLayout{typeof(+)}}) | ||
| α, A, B, β, C = M.α, M.A, M.B, M.β, M.C | ||
| if C ≡ B | ||
| B = copy(B) |
There was a problem hiding this comment.
We can change this here and elsewhere to use unaliascopy at a later date.
|
Once the |
|
Thanks, |
fix #8
It's mostly just your snippet #8 (comment) plus some tests.